Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add thousands separators to amounts #2425

Merged
merged 3 commits into from
Aug 2, 2022
Merged

Add thousands separators to amounts #2425

merged 3 commits into from
Aug 2, 2022

Conversation

matejcik
Copy link
Contributor

@matejcik matejcik commented Aug 1, 2022

fixes #2394

on trezor-core side, the thousands separators are only added to amounts, or more generally, anything formatted with format_amount. There are several more places where large integers are displayed which are not amounts. These are mostly out of scope however.

On legacy, there is no special "format_amount", so the general bn_format function is modified. I considered adding a parameter to specify whether the caller wants thousands separators or not, but decided against it in the end, as it would require changes in a lot of places and using thousands separators everywhere seems desirable anyway.

@matejcik matejcik requested review from grdddj and marnova and removed request for prusnak and onvej-sl August 1, 2022 10:03
@sime
Copy link
Contributor

sime commented Aug 1, 2022

All amounts, or all amounts that are represented as sats?

@matejcik
Copy link
Contributor Author

matejcik commented Aug 1, 2022

all amounts.

is there a requirement to not do this for non-sats?

@grdddj
Copy link
Contributor

grdddj commented Aug 1, 2022

Looking at the big negative diff - looks like all the TTui2... test-cases from fixtures.json got deleted. Was this intentional?

@matejcik
Copy link
Contributor Author

matejcik commented Aug 1, 2022

the general bn_format function is modified

this is a problem because it seems to be used internally in NEM code. it is weird that the NEM device tests did not crash on that though.

andrewkozlik
andrewkozlik reviewed Aug 1, 2022
View reviewed changes
crypto/bignum.c Outdated Show resolved Hide resolved
@sime
Copy link
Contributor

sime commented Aug 1, 2022

all amounts.

is there a requirement to not do this for non-sats?

Originally yes. Clarified with @hynek-jina that Suite will thousand separate all cryptocurrencies.

So please disregard my previous comment.

andrewkozlik
andrewkozlik reviewed Aug 1, 2022
View reviewed changes
crypto/bignum.c Outdated Show resolved Hide resolved
@matejcik
Copy link
Contributor Author

matejcik commented Aug 1, 2022

looks like all the TTui2... test-cases from fixtures.json got deleted

fixed in 3d7ea3f

@matejcik
Copy link
Contributor Author

matejcik commented Aug 1, 2022

because of the NEM thing I had to change bn_format to accept an argument after all. There is now a separate commit for crypto, and legacy makes use of it through the new bn_format_amount function that hides a bunch of parameters.
When rebasing, I will need to move the "legacy" commit after the "crypto" commit.

@andrewkozlik
Copy link
Contributor

andrewkozlik commented Aug 1, 2022 •
edited
Loading

I am wondering whether output_length is always sufficiently large to accommodate the addition of the the thousands separators. In case of bn_format_uint64() we typically allocate 32 bytes, which should always be sufficient. The most extreme example with UINT64_MAX that I can think of would be "-1,844,674,407,370,955,161.5 uDASH", which is 35 bytes including the terminating NULL, but I think it is unlikely that we could run into that considering that the maximum possible amounts in sats are not quite near UINT64_MAX.

I didn't check the size of the allocated buffers thoroughly, because I assume that the device tests would be likely to uncover any problems especially if we check the UI test screens thoroughly.

@marnova
Copy link

marnova commented Aug 1, 2022

LGTM after fixes, you just missed one fixture https://satoshilabs.gitlab.io/-/trezor/trezor-firmware/-/jobs/2797235219/artifacts/test_ui_report/failed/TT_zcash-test_sign_tx.py::test_spend_v5_input.html

@matejcik matejcik requested a review from andrewkozlik August 1, 2022 15:04
grdddj
grdddj approved these changes Aug 2, 2022
View reviewed changes
Copy link
Contributor

@grdddj grdddj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approve for the python part

@marnova
Copy link

marnova commented Aug 2, 2022

Don't forget to change the fixture for https://satoshilabs.gitlab.io/-/trezor/trezor-firmware/-/jobs/2797235219/artifacts/test_ui_report/failed/TT_zcash-test_sign_tx.py::test_spend_v5_input.html , otherwise approve

@matejcik matejcik force-pushed the matejcik/thousands branch from 3d7ea3f to 31f608e Compare August 2, 2022 13:51
@matejcik matejcik force-pushed the matejcik/thousands branch from 31f608e to b06f5b0 Compare August 2, 2022 14:39
@andrewkozlik
Copy link
Contributor

Adding thousands separators in these cases looks a bit weird:

  • #1,337
  • Memo (ID) 12,345,678,901,234

https://satoshilabs.gitlab.io/-/trezor/trezor-firmware/-/jobs/2803415060/artifacts/master_diff/diff/T1_stellar-test_stellar.py::test_sign_tx[StellarManageBuyOfferOp].html
https://satoshilabs.gitlab.io/-/trezor/trezor-firmware/-/jobs/2803415060/artifacts/master_diff/diff/T1_stellar-test_stellar.py::test_sign_tx[StellarManageSellOfferOp].html
https://satoshilabs.gitlab.io/-/trezor/trezor-firmware/-/jobs/2803415060/artifacts/master_diff/diff/T1_stellar-test_stellar.py::test_sign_tx[memo_id].html

@matejcik
Copy link
Contributor Author

matejcik commented Aug 2, 2022

Adding thousands separators in these cases looks a bit weird:

it does, but OTOH it's a correct rendering of numbers so 🤷‍♀️

@matejcik matejcik merged commit 70c1b12 into master Aug 2, 2022
@matejcik matejcik deleted the matejcik/thousands branch August 2, 2022 18:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marnova marnova marnova approved these changes

@andrewkozlik andrewkozlik andrewkozlik approved these changes

@grdddj grdddj grdddj approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Thousand amount separator
5 participants
@matejcik @sime @grdddj @andrewkozlik @marnova